8263583: Emoji rendering on macOS #3007
Conversation
|
👋 Welcome back dbatrak! A progress list of the required criteria for merging this PR into |
|
@JB-Dmitry The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
|
Looks good |
|
Initial testing it looks OK with OpenGL but it renders as garbage with the metal pipeline. |
|
/reviewers 2 |
|
@prrace |
| // This instance is used only for lookup. | ||
| static { | ||
| GraphicsPrimitiveMgr.registerGeneral( | ||
| new DrawGlyphListColor(null, null, null)); |
There was a problem hiding this comment.
If it is a complete "General" loop which supports all possible combination of incoming src/comp/dst/transform/etc I suggest to confirm that in the test.
I'm going to add Metal implementation soon. |
@mrserb This code is currently only used by GlyphListLoopPipe-based text renderers (SolidTextRenderer, AATextRenderer, LCDTextRenderer), so it won't be invoked for arbitrary composites and paint types. But different destination types (image formats) and transforms are supported. What kind of test are you having in mind? I can extend the added MacEmoji.java test, that will check that 'something' is painted to different types of BufferedImage-s and/or with transforms applied. Or you want me to add a manual test case? |
The automated tests will be enough, not sure about 'something is painted' however, it is better to check that it works. |
|
@mrserb I don't know how to check automatically that glyph painting works correctly. Could you please suggest a way to do it? In JetBrains Runtime we have a test that checks that rendered emoji glyph matches one of stored 'golden images', but I don't think that's suitable for OpenJDK, unless someone will volunteer to update that list when the need arises (e.g. when newer version of macOS changes the font, or rendering details). At the moment, we have already 7 golden images for a single glyph in our repository. |
|
You the current image from the MacEmoji test as a golden image for other formats/transforms/extraalpha/etc. For example, if DST type is changed then it is unlikely the shape of the emoji will be changed as well, or if it is too big/ too small. BTW It will be good if the test will fail on the current metal implementation. |
implementation for Metal rendering pipeline
extended test case to cover different types of target surfaces
|
Implementation for Metal pipeline has been added. I've also updated the test case to check rendering into different types of images, including VolatileImage. The test will fail now, if corresponding OpenGL (or Metal, if it's explicitly enabled) implementation part would be rolled back. |
|
Looks good! |
|
It looks fine to me. I will run the tests in CI just in case. |
|
Any chance for a second review? |
| @@ -324,6 +324,19 @@ void MTLTR_FreeGlyphCaches() { | |||
| } | |||
| } | |||
|
|
|||
| MTLPaint* storedPaint = nil; | |||
There was a problem hiding this comment.
Added 'static', thanks.
|
@JB-Dmitry This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 1005 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
add 'static' modifier to a variable
|
@mrserb Could you please re-approve the request? There's been only a tiny change since you've approved it earlier. Do I understand correctly that this re-approval is required to integrate the change? Github shows the request as having two approvals, and the 'ready' tag. Or is it just openjdk bot not implementing the workflow correctly? |
|
It is not necessary to get reapprove for push, it is just a good style to do. |
|
/integrate |
openjdk
bot
added
integrated
|
@JB-Dmitry Since your change was applied there have been 1175 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 236bd89. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
This is the implementation used by JetBrains Runtime for the last 4 years, after some cleanup, and with one problem,
found while preparing the pull request, fixed.
Even though typical scenarios for a UI application should be covered, it's not a complete solution. In particular, emoji-s
still won't be rendered for large font sizes (more than 100pt), and for non-trivial composite/painting modes.
Notable implementation details are listed below.
Glyph image generation
Deprecated CGContextShowGlyphsAtPoint function, used by JDK on macOS to render text, cannot render emojis,
CTFontDrawGlyphs is used instead. It ignores the scale component of text transformation matrix, so a 'real-sized'
CTFont object should be passed to it. The same sizing procedure is done when calculating glyph metrics, because they
are not scaled proportionally with font size (as they do for vector fonts).
Glyph image storage
Existing GlyphInfo structure is used to store color glyph image. Color glyph can be distinguished by having 4 bytes
of storage per pixel. Color components are stored in pre-multiplied alpha format.
Glyph rendering
Previously, GlyphList instance always contained glyphs in the same format (solid, grayscale or LCD), determined by the
effective rendering hint. Now the renderers must be prepared to GlyphList having 'normal' glyphs interspersed with
color glyphs (they can appear due to font fallback). This isn't a problem for OpenGL renderer (used for on-screen painting),
but GlyphListLoopPipe-based renderers (used for off-screen painting) needed an adjustment to be able to operate on
specific segments of GlyphList.
As an incidental optimization, calculation of GlyphList bounds ('getBounds' method) is performed now only when needed
(most text renderers don't need this information).
Speaking of the actual rendering of the glyph image, it's done by the straightforward glDrawPixels call in OpenGL renderer,
and by re-using existing Blit primitive in off-screen renderers.
Testing
There's no good way to test the new functionality automatically, but I've added a test verifying that 'something' is
rendered for the emoji character, when painting to BufferedImage.
Existing tests pass after the change.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3007/head:pull/3007$ git checkout pull/3007Update a local copy of the PR:
$ git checkout pull/3007$ git pull https://git.openjdk.java.net/jdk pull/3007/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 3007View PR using the GUI difftool:
$ git pr show -t 3007Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3007.diff